Repository navigation
fix(concerto-core): support AST objects in addModelFiles and updateModelFile (#1625) - #1626
Atharva15007 wants to merge 3 commits into
Conversation
…delFile (accordproject#1625) Signed-off-by: Atharva15007 <datharva157@gmail.com>
There was a problem hiding this comment.
Thanks for this fix, Atharva15007. Routing AST objects through processFile in addModelFiles, updateModelFile and validateModelFile looks correct. Existing ModelFile instances still pass through unchanged, and passing null definitions for AST input matches what addModel does. CI is green. To make this mergeable, please strengthen the tests so they would fail if the new branch did nothing (see the should-fix below). The other two points are small improvements.
Nothing blocking; some suggestions below. Findings: 1 should-fix, 2 nit.
- nit · tests ·
packages/concerto-core/test/astmodelmanager.js: Some assertions are weak.modelManager.getType(...).should.not.be.nullcan never fail becausegetTypethrows when a type is missing, and the.should.throw()in the update test has no error type or message (TypeNotFoundExceptionwould be more precise). In#addModelFiles, consider asserting the declarations are registered, e.g.getType('org.example.test1@1.0.0.Concept1'). A test thataddModelFilesrolls back when a later AST is invalid would also be valuable, since the branch change affects every non-string input. - nit · docs ·
packages/concerto-core/src/basemodelmanager.ts: The JSDoc@paramtypes forvalidateModelFile,updateModelFileandaddModelFilesstill saystring|ModelFileorstring[]|ModelFile[], while the behaviour (andModelFileInput) now also accepts an AST object. Please update them so the generated API docs match.
Automated review by Mira, an Accord Project agent, at 625cb08. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.
… and JSDocs Signed-off-by: Atharva15007 <datharva157@gmail.com>
|
Addressed in latest commit: added the negative validation test for undeclared types, strengthened assertions with TypeNotFoundException, added rollback testing for addModelFiles, and updated JSDoc @PARAM annotations. |
|
Thanks for the fix and the tests, which cover the AST paths in What happens next: the CI runs on this PR are waiting for a maintainer's approval, because the PR comes from a fork. A maintainer will read the change before approving them. We have asked the Concerto maintainers to review it. We can't give a date for a decision yet. Drafted by an Accord Project agent, reviewed by a maintainer. |
There was a problem hiding this comment.
Thanks for the update, Atharva15007. At this head addModelFiles, updateModelFile and validateModelFile accept AST objects as well as strings and ModelFile instances, passing null definitions for AST input. The earlier review points are addressed: the tests now assert on getType, check TypeNotFoundException with its message, cover rollback in addModelFiles, and the JSDoc is updated. I found nothing blocking. The one nit below is optional, and the change is otherwise ready for maintainer review.
Nothing blocking; some suggestions below. Findings: 1 nit.
Automated review by Mira, an Accord Project agent, at d5ef1d4. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.
… ModelManager.processFile Signed-off-by: Atharva15007 <datharva157@gmail.com>
Closes #1625
Changes
Flags
-None
Screenshots or Video
-N/A
Related Issues
Author Checklist
--signoffoption of git commit.mainfromfork:branchname